-
Notifications
You must be signed in to change notification settings - Fork 12
Add info message scope #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hm, with this new option, you might as well have luck making it dynamic, i.e. allowing the user to define the code as well as the keywords to match. Would also make the code more streamlined. I'm assuming that SL can handle dict settings, but I can't think of a reason it shouldn't. |
@FichteFoll, what exactly do you mean by making it dynamic? Do you mean that the scopes ( How would the code be more streamlined? |
|
Indeed, that is what I meant. Basically, you would put errors, warnings and infos into a new mapping, generate the regular expression based in the key&value pairs and adjust the following logic accordingly by getting rid of hardcoded strings and replacing them with the dictionary keys. |
Well, I am partially lost. 😉 You can already replace the default words like this: "infos": ["Note", "Info", "Nota bene"],
"warnings": ["Warning", "Don't forget", "Potential issue"],
"errors": ["Error", "TNT", "Fatal issue"]That is, if you define any of the three scopes, the words in the particular scope defined in the Anyway, @kaste, I’ve forgotten to add the |
|
Well, you can replace the words but not define the "error types". Basically, "errors" is hardcoded in the settings and maps to the error type "error". A different "API" for the settings would be so the user could basically invent new "types" just with settings. But it's not very user friendly because a user would have to set a complete dict/mapping in their settings. We don't merge the defaults dict with the user defined dict. You can also refactor the code ("streamline") without that of course. The only problem with the code is probably the nested @tukusejssirs A short PR for the README would be nice. |
|
@kaste, PR created. As for the suggestion / feature request of @FichteFoll, I’ll leave it up to you two/others, i.e. I won’t implement that as I don’t need it. |
|
@tukusejssirs Yeah, sure |
Imo that is fine for a "small" list like this, but I also personally don't need it, which is why staying at the status quo, if even for backwards compat, is probably the way to go. |
Fixes #28